Use HPACK spelling for HTTP/2 compression#2271
Conversation
When compression is enforced, the request factory creates the default Accept-Encoding value as "gzip,deflate" for HTTP/1 compatibility. HTTP/2's HPACK static table contains "gzip, deflate", so the H2 frame writer missed the compact static entry for the generated default. Rewrite only the generated default while copying headers into HTTP/2 frames. User-supplied Accept-Encoding values keep their original spelling, and HTTP/1 output remains unchanged. Codex on behalf of Pavel Ptashyts Co-Authored-By: Codex <[email protected]>
Add a deterministic encoder test proving that the HPACK static-table spelling uses a one-byte indexed representation while the old spelling requires a literal value. Clarify that the JMH benchmark reports execution time and allocation; its return value prevents dead-code elimination but is not a separate JMH metric. Codex on behalf of Pavel Ptashyts Co-Authored-By: Codex <[email protected]>
7cb1653 to
44208e5
Compare
Replace the remaining non-ASCII punctuation in the touched HPACK benchmark so the source follows the repository guidelines in AGENTS.md. Codex on behalf of Pavel Ptashyts Co-Authored-By: Codex <[email protected]>
44208e5 to
527368f
Compare
| private static CharSequence http2HeaderValue(CharSequence name, CharSequence value, boolean preferHpackAcceptEncoding) { | ||
| if (preferHpackAcceptEncoding | ||
| && HttpHeaderNames.ACCEPT_ENCODING.contentEqualsIgnoreCase(name) | ||
| && GZIP_DEFLATE.contentEquals(value)) { |
There was a problem hiding this comment.
Since GZIP_DEFLATE is a singleton AsciiString and NettyRequestFactory stores that exact reference when it calls headers.set(ACCEPT_ENCODING, GZIP_DEFLATE), you can check value == GZIP_DEFLATE here instead of contentEquals. I checked and the reference does survive into the iterator, and a user typed string with the same text is never the same reference, so identity alone tells you whether this is the auto generated header without needing the separate preferHpackAcceptEncoding check at all. That would let you delete preferHpackAcceptEncoding and the getCurrentRequest call above, which removes a second read of state that has to stay in sync with what actually built the headers.
| // Copy the HTTP/1.1 headers, dropping connection-specific names forbidden in HTTP/2 (RFC 7540 | ||
| // §8.1.2.2). iteratorCharSequence() avoids the per-name String the String-typed iterator forces; | ||
| // see isHttp2ExcludedHeader and toLowerCaseHeaderName for the skip-check and lowercasing rules. | ||
| boolean preferHpackAcceptEncoding = preferHpackAcceptEncoding(future.getCurrentRequest()); |
There was a problem hiding this comment.
This reads the policy from future.getCurrentRequest() while the header values a few lines below come from future.getNettyRequest(). They happen to always be set together today in newNettyRequestAndResponseFuture, so this works, but it is two different reads off the same future that have to stay aligned. If you switch to the identity check on GZIP_DEFLATE mentioned below this whole method and this line can go away.
| public final class NettyRequestSender { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(NettyRequestSender.class); | ||
| private static final AsciiString HTTP2_HPACK_GZIP_DEFLATE = |
There was a problem hiding this comment.
Consider moving this next to HttpUtils.GZIP_DEFLATE in HttpUtils rather than keeping it here. They are really the same concept, the HPACK static table spelling of the same value, and keeping them in separate files makes it easier for someone to change one and forget the other later. A short comment noting this is RFC 7541 appendix A static table entry 16 would also help since that context is currently only in the benchmark javadoc, not on the field itself.
| .get(30, SECONDS); | ||
|
|
||
| assertEquals(200, response.getStatusCode()); | ||
| assertEquals("gzip, deflate", response.getHeader("X-accept-encoding")); |
There was a problem hiding this comment.
response.getHeader only returns the first value for a repeated header. When compressionEnforced is true and Brotli or Zstd are available on the classpath, AHC actually sends three separate Accept-Encoding entries, not one comma joined value. Worth adding a test that runs with those codecs available and asserts on response.getHeaders("X-accept-encoding") for all values, so a rewrite that accidentally touched or duplicated the br or zstd entries would actually get caught.
| assertEquals(200, response.getStatusCode()); | ||
| assertEquals("gzip,deflate", response.getHeader("X-accept-encoding")); | ||
| } | ||
| } |
There was a problem hiding this comment.
These two tests only cover HTTP/2. Since the whole point of the change is that HTTP/1 output stays as gzip,deflate with no space, it would be worth adding a small HTTP/1 test with compressionEnforced true asserting the header is still sent without the space, so that contract is pinned instead of just asserted in the PR description.
| assertEquals(1, encodedLength("gzip, deflate")); | ||
| } | ||
|
|
||
| private static int encodedLength(String value) throws Exception { |
There was a problem hiding this comment.
This test builds headers by hand and drives Netty's encoder directly, it does not go through AHC's own request building or NettyRequestSender at all. It is a good proof that the two spellings differ in encoded size, but it is not proof that AHC actually produces the gzip, deflate spelling in production. That part is only covered by the two BasicHttp2Test cases. Might be worth a one line comment on the class saying that explicitly so nobody later assumes this test exercises the real code path.
2 participants
Summary
Accept-Encodingvalue fromgzip,deflatetogzip, deflatewhile copying headers into HTTP/2 framesAccept-Encodingspelling and leave HTTP/1 output unchangedPerformance evidence
With Netty 4.2.15.Final and a fresh HPACK encoder:
gzip,deflate: 14 encoded bytesgzip, deflate: 1 encoded byte through the indexed static-table representationA short JMH validation on JDK 21 with
-prof gcmeasured 1032 B/op for the literal spelling and 680 B/op for the static-table spelling. Timing was noisy, so this PR relies on the deterministic wire-size test rather than claiming a stable CPU improvement.Validation
./mvnw -pl client -Dtest='org.asynchttpclient.netty.request.AcceptEncodingHpackTest,org.asynchttpclient.BasicHttp2Test#generatedAcceptEncodingUsesHpackStaticValueOverHttp2+userAcceptEncodingSpellingIsPreservedOverHttp2' test(3 tests)AcceptEncodingHpackBenchmark -f 1 -wi 3 -i 5 -w 500ms -r 500ms -prof gcBasicHttp2Testvalidation from the initial changeAttribution
Codex on behalf of Pavel Ptashyts